Fix a missing Plans column on the applications list #3995

mayorova · 2025-01-28T11:52:26Z

What this PR does / why we need it:

Fixes the missing Plan column on the application listing.

Before:

After:

Which issue(s) this PR fixes

not registered

Verification steps

View a list of applications under a service that has multiple plans, and make sure that there is a Plans column.

Special notes for your reviewer:

See comments inline

mayorova · 2025-01-28T11:54:32Z

app/controllers/api/applications_controller.rb

  before_action :find_service
+  before_action :find_plans


This order is important for this controller, as when the plans are searched, they are scoped by the service, so the service needs to be pre-filled.

mayorova · 2025-01-28T11:57:17Z

app/controllers/api/applications_controller.rb

@@ -42,8 +42,8 @@ def find_service
    @service = accessible_services.find params[:service_id]
  end

-  def accessible_plans
-    super.where(issuer: @service)
+  def find_plans


Overriding the find_plans in order to avoid adding .stock.
As I understand the logic, the idea is that if there is just one single plan (I guess that's not commonly used, but still), there is no need to show the Plan column, because well, it will always be the same.

But for that purpose I think it is more correct to check all plans (including the custom ones), and not just "stock" plans (non-custom).

mayorova · 2025-01-28T12:02:44Z

app/views/shared/applications/_listing.html.slim

-/ FIXME: The first condition does not make any sense but application_plans used to be accessible_plans, now it's accessible_plans.stock
-/ FIXME 2: application_plans returns [] in services/:id/applications even though there are plans
- show_application_plans = application_plans.size > 0 && !master_on_premises?
+- show_application_plans = application_plans.size > 1 && !master_on_premises?


I agree that the condition didn't make sense 😅
And I think originally it was > 1, according to this, it was changed here.

I believe this PR is restoring the originally intended logic.

mayorova · 2025-01-28T12:03:07Z

features/api/services/applications/index.feature

@@ -6,8 +6,8 @@ Feature: Product > Applications

  Background:
    Given a provider
-    And a product "My API"
-    And another product "Another API"
+    And a product "My API" with no plans


Just to make sure that the plans created below are the only plans on the respective services.

mayorova · 2025-01-28T12:55:56Z

features/step_definitions/table_steps.rb

@@ -21,7 +21,7 @@
 end

 # TODO: can we use "has_table?" instead of this complex step?
-Then "(I )(they )should see (the )following table(:)" do |expected|
+Then /^(?:I )?(?:they )?should see (?:the )?following table( with exact columns)?:?$/ do |exact_columns, expected|


The step was change to avoid touching all the existing scenarios.
I couldn't find a way to add a parameter type within an optional block... In fact, I tried, but cucumber said it was not possible.

Not possible, I've wanted to do the same many times.
However, this is a bit redundant as there is already a way to do this:

Then the table has a column "Plan" But the table does not have a column "Plan"

If you still want to keep it I would simply suggest:

Suggested change

Then /^(?:I )?(?:they )?should see (?:the )?following table( with exact columns)?:?$/ do |exact_columns, expected|

Then /^(?:I |they )?should see (?:the )?following table( with exact columns)?:?$/ do |exact_columns, expected|

mayorova · 2025-01-28T14:37:14Z

features/step_definitions/table_steps.rb

@@ -40,7 +40,8 @@

  retries ||= 1

-  expected.diff! table
+  options = exact_columns.present? ? { surplus_col: true } : {}


With the surplus_col option, the comparison will fail if the actual table has an extra column that is not defined in the expected table.

josemigallas · 2025-01-29T07:49:29Z

features/api/services/applications/index.feature

-      | Bob's App       | live  | Bob     | December 11, 2023 |            |
+    When they go to product "My API" applications page
+    Then they should see the following table with exact columns:
+      # Plan column is visible because this service has multiple plans


These comments tell me that these scenarios have double intent. I suggest we keep them as they were and add a new scenario that tests specifically one thing, the Plans column:

Scenario: Plan column is visible when service has multiple plans Scenario: Plan column is not visible when service has just one plan

Well, the intention is to verify that the table is rendered exactly as expected.
If the presence of the column Plans depends on some conditions, this is taken into account. I didn't know about the the table has a column step, and well, it seems useful, but I think verifying not only that the column is present (I guess it checks the header), but also that each row has a correct value for that column is important.

I can remove those comment lines 😬 And then this verification would be kind of implicit.

It's a matter of not mixing concerns. Let me elaborate.

The scenario reads "Only the current service applications are listed", it meets specific conditions and performs specific assertions. There are 2 products, each own with plans, and I want to make sure that the index page doesn't mix up both.

This PR implements (or rather fixes) a completely different piece of logic: that depending on the number of plans (regardless of the current service), there will be an extra column.

Testing that "the table renders exactly as expected" as you put it isn't ideal because nobody will now what "as expected" means. It may be clear now in this PR, but anybody looking at it without context will get confused. If this test ever fails, it will require some degree of debugging and investigation to figure out that column Plan's visibility changes.

This should be made explicit by the scenario's title, background and steps.

This of course also applies to the next scenario: "Paid? column is shown when finance is enabled". There is a whole scenario to test column "Paid?", why not for "Plans"?

Say someone adds a new plan to "Another API", for whatever reason. Then Paid? column is shown when finance is enabled and Only the current service applications are listed will both fail, even though they should not, because "Another API" having 2 plans doesn't affect either the column "Paid?" or "API"'s applications table.

I don't disagree with anything you say, but I am not sure what is the best way to "make this right".

Testing that "the table renders exactly as expected" as you put it isn't ideal because nobody will now what "as expected" means.

Well, in this case "as expected" means - exactly like the table in the step. I was actually surprised that the the expected table didn't need to be complete - I think this is quite error-prone. An accidental change may result in some columns disappearing, and we'll never catch it.

it will require some degree of debugging and investigation to figure out that column Plan's visibility changes.

That was the reason why I added those comments, although I agree, this is not ideal, and the scenario/steps descriptions should be enough to explain what the test is verifying.

This of course also applies to the next scenario: "Paid? column is shown when finance is enabled". There is a whole scenario to test column "Paid?", why not for "Plans"?

This is also not ideal, because this pretty much copies the previous scenario, but just adds the "Paid?" column. And this is the reason why it has to be a separate scenario - because it can't be tested together with the previous one.

@josemigallas I've updated the scenarios here: 6b718f7

mayorova commented Jan 28, 2025

View reviewed changes

mayorova force-pushed the fix-showing-plans-on-service-apps-list branch 2 times, most recently from 0276a85 to 79e1076 Compare January 28, 2025 12:54

mayorova commented Jan 28, 2025

View reviewed changes

mayorova force-pushed the fix-showing-plans-on-service-apps-list branch from bc527f6 to 9793901 Compare January 28, 2025 14:34

mayorova commented Jan 28, 2025

View reviewed changes

josemigallas reviewed Jan 29, 2025

View reviewed changes

Fix a missing Plans column on the applications list

d5c1e11

mayorova force-pushed the fix-showing-plans-on-service-apps-list branch from 9793901 to d5c1e11 Compare January 29, 2025 09:31

Make application list scenarios more explicit

6b718f7

mayorova force-pushed the fix-showing-plans-on-service-apps-list branch from bee4d4d to 6b718f7 Compare January 29, 2025 11:27

josemigallas approved these changes Feb 3, 2025

View reviewed changes

mayorova merged commit bcf3e0b into master Feb 3, 2025
17 of 21 checks passed

mayorova deleted the fix-showing-plans-on-service-apps-list branch February 3, 2025 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a missing Plans column on the applications list #3995

Fix a missing Plans column on the applications list #3995

mayorova commented Jan 28, 2025 •

edited

Loading

mayorova Jan 28, 2025

mayorova Jan 28, 2025

mayorova Jan 28, 2025

mayorova Jan 28, 2025

mayorova Jan 28, 2025

josemigallas Jan 29, 2025 •

edited

Loading

mayorova Jan 28, 2025

josemigallas Jan 29, 2025 •

edited

Loading

mayorova Jan 29, 2025

josemigallas Jan 29, 2025

mayorova Jan 29, 2025

mayorova Jan 29, 2025

	Then /^(?:I )?(?:they )?should see (?:the )?following table( with exact columns)?:?$/ do \|exact_columns, expected\|
	Then /^(?:I \|they )?should see (?:the )?following table( with exact columns)?:?$/ do \|exact_columns, expected\|

Fix a missing Plans column on the applications list #3995

Fix a missing Plans column on the applications list #3995

Conversation

mayorova commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemigallas Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Jan 28, 2025 •

edited

Loading

josemigallas Jan 29, 2025 •

edited

Loading

josemigallas Jan 29, 2025 •

edited

Loading